Fixes spin_until_future_complete removing node it didn't add #1316

jmblixt3 · 2024-07-18T01:22:54Z

Closes #1313
Current if spin_unitl_future_complete is called inside a nodes callback it removes the node from the executor
This results in any subsiquent waitables to never be checked by the node since the node is no longer in the executor
This aims to fix that by only removing the node from the executor if it wasn't already present

fujitatomoya

@jmblixt3

does this really resolve the #1313 (comment)? i think you are using the same callback group, and rcl_wait with recursive executor spin. seems pretty much you are hitting the situation described in https://docs.ros.org/en/rolling/How-To-Guides/Using-callback-groups.html#avoiding-deadlocks?

even if that works, waiting on the future completion synchronously in the callback would block the other entities with the same callback group to be blocked, i would not think this is good design practice for the application.

i may be missing something, i would like to get more comments from others.

jmblixt3 · 2024-07-24T02:31:00Z

Looking at rclcpp clcpp/executors.hpp, there's a similar TODO to the issue I described. I'm not sure which version of test code you looked at, but this is what I used to validate the issue beforehand, and how to show it was fixed after. Looking at the original issue see the usage of fibonacci_action_server, fibonacci_action_hybrid, and ros2 action send_goal. The last version of the hybrid is below

fibomnacci_action_hybrid.py

import rclpy
from rclpy import Future
from rclpy.action import ActionServer, ActionClient
from rclpy.action.client import ClientGoalHandle
from rclpy.action.server import ServerGoalHandle
import rclpy.logging
from rclpy.node import Node

from action_tutorials_interfaces.action import Fibonacci

from rclpy.executors import ExternalShutdownException

class FibonacciActionServer(Node):
    def __init__(self):
        super().__init__("fibonacci_action_server")
        # self.client_node = rclpy.create_node('fibonacci_action_server_client')
        self._action_server = ActionServer(
            self, Fibonacci, "some_action", self.execute_callback
        )
        self._action_client = ActionClient(
            self, Fibonacci, "fibonacci"
        )

    def execute_callback(self, goal_handle: ServerGoalHandle):
        self.get_logger().info("Requesting goal...")
        request_future = self._action_client.send_goal_async(Fibonacci.Goal(order=goal_handle.request.order))
        rclpy.spin_until_future_complete(self,request_future)
        self.get_logger().info("Recieved request result...")
        spin_handle: ClientGoalHandle = request_future.result()
        result_future: Future = spin_handle.get_result_async()
        rclpy.spin_until_future_complete(self,result_future)
        self.get_logger().info("Recieved final result...")
        result = Fibonacci.Result()
        if not result_future.cancelled():
            result = result_future.result().result
        goal_handle.succeed()
        return result



def main(args=None):
    try:
        with rclpy.init(args=args):
            fibonacci_action_server = FibonacciActionServer()
            rclpy.spin(fibonacci_action_server)
    except (KeyboardInterrupt, ExternalShutdownException):
        pass


if __name__ == '__main__':
    main()

Closes rclpy:ros2#1313 Current if spin_unitl_future_complete is called inside a nodes callback it removes the node from the executor This results in any subsiquent waitables to never be checked by the node since the node is no longer in the executor This aims to fix that by only removing the node from the executor if it wasn't already present Signed-off-by: Jonathan Blixt <[email protected]>

MichaelOrlov · 2024-08-22T17:55:09Z

@sloretz It seems we did talk about this issue before. Could you please follow up on it and give some comments?

sloretz

The fix LGTM, but please use coroutines for waiting for results inside a callback #1313 (comment)

sloretz · 2024-08-23T18:40:47Z

@Mergifyio update

mergify · 2024-08-23T18:40:53Z

update

✅ Branch has been successfully updated

sloretz · 2024-08-23T18:41:21Z

Pulls: #1316
Gist: https://gist.githubusercontent.com/sloretz/450ced2a382151503e56242c22eeccf7/raw/cae0f91ede29d09f58213d1e855db468c8081b45/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14465

Linux (Infra issue; rerun: )
Linux-aarch64
Linux-rhel
Windows

sloretz · 2024-08-24T00:06:00Z

Thanks for the PR!

jmblixt3 · 2024-08-24T00:08:32Z

@sloretz can this be back ported?

sloretz · 2024-08-24T00:27:36Z

@Mergifyio backport jazzy iron humble

mergify · 2024-08-24T00:27:52Z

backport jazzy iron humble

✅ Backports have been created

#1341 Fixes spin_until_future_complete removing node it didn't add (backport #1316) has been created for branch jazzy
#1342 Fixes spin_until_future_complete removing node it didn't add (backport #1316) has been created for branch iron
#1343 Fixes spin_until_future_complete removing node it didn't add (backport #1316) has been created for branch humble

Closes rclpy:#1313 Current if spin_unitl_future_complete is called inside a nodes callback it removes the node from the executor This results in any subsiquent waitables to never be checked by the node since the node is no longer in the executor This aims to fix that by only removing the node from the executor if it wasn't already present Signed-off-by: Jonathan Blixt <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 47346ef)

Closes rclpy:#1313 Current if spin_unitl_future_complete is called inside a nodes callback it removes the node from the executor This results in any subsiquent waitables to never be checked by the node since the node is no longer in the executor This aims to fix that by only removing the node from the executor if it wasn't already present Signed-off-by: Jonathan Blixt <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 47346ef) Co-authored-by: Jonathan <[email protected]>

jmblixt3 requested review from sloretz and adityapande-1995 as code owners July 18, 2024 01:22

Barry-Xu-2018 mentioned this pull request Jul 18, 2024

Incompatibility with calling spin_until_future_complete while in callback #1313

Closed

fujitatomoya reviewed Jul 23, 2024

View reviewed changes

mjcarroll requested a review from cottsay August 8, 2024 14:04

jmblixt3 force-pushed the 1313-spin-executor-fix branch from 05f17ee to f32a86c Compare August 13, 2024 20:42

sloretz changed the title ~~Fixes spin_until_future_complete inside callback~~ Fixes spin_until_future_complete removing node it didn't add Aug 23, 2024

sloretz approved these changes Aug 23, 2024

View reviewed changes

Merge branch 'rolling' into 1313-spin-executor-fix

e177054

sloretz merged commit 47346ef into ros2:rolling Aug 24, 2024
3 checks passed

mergify bot mentioned this pull request Aug 24, 2024

Fixes spin_until_future_complete removing node it didn't add (backport #1316) #1341

Merged

mergify bot mentioned this pull request Aug 24, 2024

Fixes spin_until_future_complete removing node it didn't add (backport #1316) #1342

Merged

mergify bot mentioned this pull request Aug 24, 2024

Fixes spin_until_future_complete removing node it didn't add (backport #1316) #1343

Merged

fujitatomoya mentioned this pull request Aug 28, 2024

ROS2 Iron: Action Servers Become Unresponsive After Modifying Collision Checking #1337

Open

jmblixt3 deleted the 1313-spin-executor-fix branch September 15, 2024 15:49

jmblixt3 mentioned this pull request Sep 15, 2024

async wait/sleep implementation #1234

Open

fujitatomoya mentioned this pull request Oct 3, 2024

Action server does not receive goals after the first one has terminated ros2/ros2#1609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes spin_until_future_complete removing node it didn't add #1316

Fixes spin_until_future_complete removing node it didn't add #1316

jmblixt3 commented Jul 18, 2024

fujitatomoya left a comment

jmblixt3 commented Jul 24, 2024

MichaelOrlov commented Aug 22, 2024

sloretz left a comment

sloretz commented Aug 23, 2024

mergify bot commented Aug 23, 2024

sloretz commented Aug 23, 2024 •

edited

Loading

sloretz commented Aug 24, 2024

jmblixt3 commented Aug 24, 2024

sloretz commented Aug 24, 2024

mergify bot commented Aug 24, 2024 •

edited

Loading

Fixes spin_until_future_complete removing node it didn't add #1316

Fixes spin_until_future_complete removing node it didn't add #1316

Conversation

jmblixt3 commented Jul 18, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

jmblixt3 commented Jul 24, 2024

fibomnacci_action_hybrid.py

MichaelOrlov commented Aug 22, 2024

sloretz left a comment

Choose a reason for hiding this comment

sloretz commented Aug 23, 2024

mergify bot commented Aug 23, 2024

✅ Branch has been successfully updated

sloretz commented Aug 23, 2024 • edited Loading

sloretz commented Aug 24, 2024

jmblixt3 commented Aug 24, 2024

sloretz commented Aug 24, 2024

mergify bot commented Aug 24, 2024 • edited Loading

✅ Backports have been created

sloretz commented Aug 23, 2024 •

edited

Loading

mergify bot commented Aug 24, 2024 •

edited

Loading